Redact environment variable values in vminitd debug logs - #813
Conversation
|
Can you rebase on |
cec005d to
50f7722
Compare
|
Rebased onto The rebase was content-free: Two things worth flagging, both my doing:
|
|
Hi @crosbymichael, Following up on the rebase. Both workflows are still at action_required on That approval is the only thing blocking. No rush, just flagging it. |
5424632 to
c339953
Compare
|
@vyncint how do you think we can prevent future code from reintroducing this issue? |
Hi @crosbymichael, The current patch is opt-in, so a new log line would reintroduce the leak. I would |
|
@vyncint that sounds great. I think that is the correct move. |
vminitd logs the OCI spec and the exec process at debug level, so every NAME=value environment entry ended up in the boot log (apple#518). Environment variables routinely carry secrets. Conform Process and Hook to CustomStringConvertible with a rendering that masks environment values and keeps the names. Spec and Hooks inherit it, because reflection renders a nested value through its own description, so interpolating a spec is redacted whether or not the author knows to ask for it. The two existing log sites need no change. Codable is untouched: an encoded spec still carries the real values, and process.env still returns them.
c339953 to
151dfc3
Compare
|
Thanks. Pushed that version.
One gap worth naming: The workflows are still at |
|
All checks are green on 151dfc3, including the Linux compile check and the signature verification. |
|
Thanks for working through this with me! |
2 participants
Fixes #518.
What
vminitd logs the full OCI spec and exec process at debug level in
ManagedContainer("created bundle with spec …", "creating exec process with …"), which puts everyNAME=valueenvironment entry into the boot log. Environment variables routinely carry secrets, socontainer logs --boot web | grep PASSWORDreproduces the leak exactly as described in #518.Rather than redacting at the call sites, this makes the redacted form the default rendering of the types that own an environment:
ProcessandHookconform toCustomStringConvertiblewith values masked and names kept.SpecandHooksinherit it, because Swift's reflection-based description renders a nested value through that value's owndescription.The effect is that any
\(spec)or\(process)is safe without the author knowing this file exists, which is what stops a log line added later from reintroducing the leak. The two existing log sites are unchanged, so this no longer touches vminitd at all.Two details worth calling out:
Codableis untouched.descriptiongoverns text rendering only, so an encoded spec still carries the real values and nothing changes about what is written to disk or sent to the guest. The unredacted environment also remains available to callers throughprocess.env.descriptionrenders through a mirror rather than a hand-written field list.Processhas 13 fields; listing them by hand would drop the rest from the log line and would rot as fields are added.Verification
SpecRedactionTests(9 tests) cover: a wholeSpecinterpolated into a log line never renders the values;String(describing:)andString(reflecting:)are redacted too; variable names survive;NAME-only inherit entries pass through;NAME=and values containing further=are masked whole; encoding round-trips with the real values; rendering does not mutate; and the other fields are still rendered.ContainerizationOCITestspasses, 58 tests in 9 suites.swift format lint --strict --configuration .swift-format-nolintis clean, andswift formatleaves both files unchanged.Every line here is one I can explain and justify; the reasoning above is the complete rationale for each change.